feature/capr 30 create global error handler - #67

Please address the comments from this code review:

## Overall Comments
- The new `capy_discord.instance` deprecation shim looks incorrect: assigning `capy_discord.instance = Bot(...)` in `__main__` creates a real module attribute and bypasses `__getattr__`, so the deprecation warning never fires and `_instance` is never updated—if you want to keep compatibility while warning, consider keeping a real `instance` attribute and emitting the warning from a helper or at assignment time instead of via `__getattr__`.
- The `_error_test.ErrorTest` cog’s raised exceptions (`ValueError("Generic error")`, `UserFriendlyError("Log", "User message")`) don’t match the messages used in the tests and description (e.g. tests expect "Generic Test Error" / "Internal Error Log" / "User Message"), which will cause brittle or failing assertions—align the exception messages (and types if needed) with the intended test expectations.

## Individual Comments

### Comment 1
<location> `capy_discord/__init__.py:14-23` </location>
<code_context>
+_instance: Bot | None = None
+
+
+def __getattr__(name: str) -> object:
+    if name == "instance":
+        warnings.warn(
+            "capy_discord.instance is deprecated. Use dependency injection.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        return _instance
+
+    msg = f"module {__name__!r} has no attribute {name!r}"
+    raise AttributeError(msg)
</code_context>

<issue_to_address>
**issue (bug_risk):** The deprecation mechanism for `capy_discord.instance` is bypassed by normal attribute assignment and `_instance` is never updated.

Because `__main__.py` assigns `capy_discord.instance = Bot(...)`, it creates a real module attribute and `__getattr__` is no longer called for `instance`. As a result, the deprecation warning only fires before the first assignment (if ever), and `_instance` is never updated, so it cannot be treated as the canonical store.

To align behavior with the intended deprecation path, you could either:
- Add a `__setattr__` (or dedicated setter) that updates `_instance` and emits the warning on write, or
- Make `instance: Bot | None = None` a normal module attribute and emit the warning through property-like accessors, avoiding `_instance` entirely.
</issue_to_address>

### Comment 2
<location> `capy_discord/ui/views.py:60` </location>
<code_context>
         interaction: discord.Interaction,
         content: str | None = None,
-        embed: discord.Embed | None = None,
+        embed: discord.Embed = discord.utils.MISSING,
         embeds: list[discord.Embed] = discord.utils.MISSING,
         file: discord.File = discord.utils.MISSING,
</code_context>

<issue_to_address>
**suggestion:** The `embed` parameter type no longer reflects the actual values (including `MISSING` and `None`) that can be passed through.

With the new default, `embed` can now be `discord.Embed`, `discord.utils.MISSING`, or `None` (if explicitly passed), but the annotation only reflects `discord.Embed`. Static type checkers will assume `embed` is never `None`/`MISSING`. Consider updating the annotation to something like `discord.Embed | None | discord.utils.MissingType` (or a local alias) so it matches the actual possible values.

Suggested implementation:

```python
        interaction: discord.Interaction,
        content: str | None = None,
        embed: discord.Embed | None | discord.utils.MissingType = discord.utils.MISSING,
        embeds: list[discord.Embed] = discord.utils.MISSING,
        file: discord.File = discord.utils.MISSING,

```

If you prefer not to reference `discord.utils.MissingType` inline, you could:
1. Import `MissingType` (e.g. `from discord.utils import MissingType`), and
2. Change the annotation to `discord.Embed | None | MissingType`.
Also consider applying the same pattern to other parameters that use `discord.utils.MISSING` as a default (e.g. `embeds`, `file`, `files`) so their type hints match their actual possible values.
</issue_to_address>

### Comment 3
<location> `capy_discord/bot.py:41-43` </location>
<code_context>
+                await interaction.response.send_message(embed=embed, ephemeral=True)
+            return
+
+        # Generic error handling
+        logger = self._get_logger_for_command(interaction.command)
+        logger.exception("Slash command error: %s", error)
+        embed = error_embed(description="An unexpected error occurred. Please try again later.")
+        if interaction.response.is_done():
</code_context>

<issue_to_address>
**suggestion:** The logged error message uses the wrapper exception instead of the unwrapped `actual_error`, which may reduce log clarity.

In `on_tree_error` and `on_command_error` you unpack `CommandInvokeError` into `actual_error`, but still log `error`. Please switch the format argument to `actual_error` so the underlying exception type/message is visible, e.g.:
```python
logger.exception("Slash command error: %s", actual_error)
```
The same applies to the prefix command handler.

Suggested implementation:

```python
        # Generic error handling
        logger = self._get_logger_for_command(interaction.command)
        logger.exception("Slash command error: %s", actual_error)
        embed = error_embed(description="An unexpected error occurred. Please try again later.")

```

You should also update the prefix command handler (`on_command_error`) in the same way. Wherever you have a pattern roughly like:

```python
actual_error = error.original
logger.exception("Command error: %s", error)
```

change it to:

```python
actual_error = error.original
logger.exception("Command error: %s", actual_error)
```

so that the underlying exception type and message are logged consistently for both slash and prefix commands.
</issue_to_address>

### Comment 4
<location> `tests/capy_discord/exts/test_error_test_cog.py:22-24` </location>
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_error_test_generic(cog):
+    interaction = MagicMock(spec=discord.Interaction)
+    with pytest.raises(ValueError, match="Generic Test Error"):
+        await cog.error_test(interaction, "generic")
+
</code_context>

<issue_to_address>
**issue (testing):** The expectations in these tests don’t match the current ErrorTest cog messages and will fail as written.

`ErrorTest` currently raises `ValueError("Generic error")` and `UserFriendlyError("Log", "User message")`, but these tests expect `"Generic Test Error"` and `"Internal Error Log"`. As written, they will always fail. Please either update the `match=` patterns (and any related expectations) to reflect the current messages, or change the implementation if the test expectations represent the intended contract.
</issue_to_address>

### Comment 5
<location> `tests/capy_discord/exts/test_sync.py:24-31` </location>
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_sync_command_error_bubbles(cog, bot):
+    ctx = MagicMock(spec=commands.Context)
+    ctx.bot = bot
+    ctx.author.id = 123
+    ctx.send = AsyncMock()
+    bot.tree.sync.side_effect = Exception("Sync failed")
+
+    with pytest.raises(Exception, match="Sync failed"):
+        await cog.sync.callback(cog, ctx)
+
</code_context>

<issue_to_address>
**suggestion (testing):** Sync tests only cover error bubbling; consider adding success and edge-case scenarios.

These tests validate exception bubbling from `tree.sync`, but don’t exercise normal behavior of the refactored commands. Please also add:

- A happy-path test for `sync` to assert the expected description text, successful `_sync_commands` behavior, and logging.
- A happy-path test for `sync_slash` to assert `defer` is called, the followup message lists the correct commands, and logging occurs.
- `spec` edge-case tests:
  - `spec` in `{'.', 'guild'}` with `ctx.guild` set, asserting guild-specific sync and messaging.
  - `spec` in `{'.', 'guild'}` with `ctx.guild` is `None`, asserting the early return and the "must be used in a guild" message.
  - `spec = 'clear'` to cover the clear-commands branch.

This will better ensure the error-handling refactor hasn’t regressed normal sync behavior.

Suggested implementation:

```python
@pytest.fixture
def cog(bot):
    return Sync(bot)


@pytest.mark.asyncio
async def test_sync_command_error_bubbles(cog, bot):
    ctx = MagicMock(spec=commands.Context)
    ctx.bot = bot
    ctx.author.id = 123
    ctx.send = AsyncMock()
    bot.tree.sync.side_effect = Exception("Sync failed")

    with pytest.raises(Exception, match="Sync failed"):
        await cog.sync.callback(cog, ctx)


@pytest.mark.asyncio
async def test_sync_command_success(cog, bot, caplog):
    ctx = MagicMock(spec=commands.Context)
    ctx.bot = bot
    ctx.author.id = 123
    ctx.guild = None
    ctx.send = AsyncMock()

    # Make sure sync succeeds
    bot.tree.sync = AsyncMock(return_value=["cmd1", "cmd2"])

    with caplog.at_level("INFO"):
        await cog.sync.callback(cog, ctx)

    bot.tree.sync.assert_awaited_once()
    ctx.send.assert_awaited_once()
    # Relaxed assertion: just ensure we sent some success text
    sent_args, sent_kwargs = ctx.send.await_args
    assert "sync" in sent_args[0].lower()

    # Ensure we logged something about sync succeeding
    assert any("sync" in r.getMessage().lower() for r in caplog.records)


@pytest.mark.asyncio
async def test_sync_slash_success(cog, bot, caplog):
    interaction = MagicMock(spec=discord.Interaction)
    interaction.client = bot
    interaction.guild = None
    interaction.user.id = 123
    interaction.response.defer = AsyncMock()
    interaction.followup.send = AsyncMock()

    bot.tree.sync = AsyncMock(return_value=["cmd1", "cmd2"])

    with caplog.at_level("INFO"):
        await cog.sync_slash.callback(cog, interaction)

    interaction.response.defer.assert_awaited_once()
    bot.tree.sync.assert_awaited_once()

    interaction.followup.send.assert_awaited_once()
    args, kwargs = interaction.followup.send.await_args
    assert "cmd1" in args[0]
    assert "cmd2" in args[0]

    assert any("sync" in r.getMessage().lower() for r in caplog.records)


@pytest.mark.asyncio
async def test_sync_spec_guild_with_guild(cog, bot):
    ctx = MagicMock(spec=commands.Context)
    ctx.bot = bot
    ctx.author.id = 123
    ctx.guild = MagicMock()
    ctx.guild.id = 456
    ctx.send = AsyncMock()

    bot.tree.sync = AsyncMock(return_value=["gcmd"])
    await cog.sync.callback(cog, ctx, spec="guild")

    bot.tree.sync.assert_awaited_once()
    # Expect a guild-specific sync (implementation may use guild / guild_id)
    call_kwargs = bot.tree.sync.await_args.kwargs
    assert "guild" in call_kwargs or "guild_id" in call_kwargs

    ctx.send.assert_awaited_once()
    sent_args, _ = ctx.send.await_args
    assert "guild" in sent_args[0].lower()


@pytest.mark.asyncio
async def test_sync_spec_guild_without_guild(cog, bot):
    ctx = MagicMock(spec=commands.Context)
    ctx.bot = bot
    ctx.author.id = 123
    ctx.guild = None
    ctx.send = AsyncMock()

    bot.tree.sync = AsyncMock(return_value=["gcmd"])

    await cog.sync.callback(cog, ctx, spec="guild")

    # Should early-return without calling sync
    bot.tree.sync.assert_not_awaited()
    ctx.send.assert_awaited_once()
    sent_args, _ = ctx.send.await_args
    assert "must be used in a guild" in sent_args[0].lower()


@pytest.mark.asyncio
async def test_sync_spec_dot_with_guild(cog, bot):
    ctx = MagicMock(spec=commands.Context)
    ctx.bot = bot
    ctx.author.id = 123
    ctx.guild = MagicMock()
    ctx.guild.id = 456
    ctx.send = AsyncMock()

    bot.tree.sync = AsyncMock(return_value=["gcmd"])
    await cog.sync.callback(cog, ctx, spec=".")

    bot.tree.sync.assert_awaited_once()
    call_kwargs = bot.tree.sync.await_args.kwargs
    assert "guild" in call_kwargs or "guild_id" in call_kwargs

    ctx.send.assert_awaited_once()
    sent_args, _ = ctx.send.await_args
    assert "guild" in sent_args[0].lower()


@pytest.mark.asyncio
async def test_sync_spec_dot_without_guild(cog, bot):
    ctx = MagicMock(spec=commands.Context)
    ctx.bot = bot
    ctx.author.id = 123
    ctx.guild = None
    ctx.send = AsyncMock()

    bot.tree.sync = AsyncMock(return_value=["gcmd"])

    await cog.sync.callback(cog, ctx, spec=".")

    bot.tree.sync.assert_not_awaited()
    ctx.send.assert_awaited_once()
    sent_args, _ = ctx.send.await_args
    assert "must be used in a guild" in sent_args[0].lower()


@pytest.mark.asyncio
async def test_sync_spec_clear(cog, bot):
    ctx = MagicMock(spec=commands.Context)
    ctx.bot = bot
    ctx.author.id = 123
    ctx.guild = None
    ctx.send = AsyncMock()

    bot.tree.clear_commands = MagicMock()
    bot.tree.sync = AsyncMock(return_value=[])

    await cog.sync.callback(cog, ctx, spec="clear")

    bot.tree.clear_commands.assert_called_once()
    bot.tree.sync.assert_awaited_once()
    ctx.send.assert_awaited_once()
    sent_args, _ = ctx.send.await_args
    assert "cleared" in sent_args[0].lower()

```

1. Ensure the top of `tests/capy_discord/exts/test_sync.py` imports the testing utilities used in these tests:
   ```python
   from unittest.mock import AsyncMock, MagicMock
   import pytest
   import discord
   from discord.ext import commands

   from capy_discord.exts.tools.sync import Sync
   ```
   (If these are already present, avoid duplicating them.)
2. The tests assume:
   - `Sync.sync` is a regular command whose underlying callback can be invoked as `cog.sync.callback(cog, ctx, spec=None)`.
   - `Sync.sync_slash` is an application command whose callback can be invoked as `cog.sync_slash.callback(cog, interaction, spec=None)`.
   - For guild-specific sync (`spec in {'.', 'guild'}`), the implementation passes a `guild` or `guild_id` keyword to `bot.tree.sync`.
   - When a guild-only spec is used without a guild, the implementation sends a message containing "must be used in a guild".
   - For `spec="clear"`, the implementation calls `bot.tree.clear_commands()` and then `bot.tree.sync()`, and sends a message containing "cleared".
   If the actual implementation differs, adjust the assertions (especially message text and `bot.tree.sync` call-shape) to match the real behavior.
</issue_to_address>

### Comment 6
<location> `tests/capy_discord/test_error_utility.py:6-13` </location>
<code_context>
+from capy_discord.ui.embeds import error_embed
+
+
+def test_error_embed_defaults():
+    """Test error_embed with default values."""
+    description = "Something went wrong"
+    embed = error_embed(description=description)
+
+    assert embed.title == "❌ Error"
+    assert embed.description == description
+    assert embed.color == discord.Color.red()
+
+
</code_context>

<issue_to_address>
**suggestion (testing):** Nice coverage for error_embed; consider complementary tests for view-level error handling that consumes it.

Since `BaseView.on_error` now uses `error_embed` and branches on `interaction.response.is_done()`, it would be useful to add view-level tests that:

- Mock a `discord.Interaction` where `response.is_done()` returns both `False` and `True` to verify it calls `response.send_message` vs `followup.send` as expected.
- Assert that `on_error` uses `error_embed` for the embed and that the message is ephemeral.

That will complement these unit tests by covering the end-to-end error handling behavior on views.

Suggested implementation:

```python
import discord
from unittest.mock import AsyncMock, MagicMock

import pytest

from capy_discord.ui.embeds import error_embed
from capy_discord.ui.view import BaseView

```

```python
    embed = error_embed(description=description)

    assert embed.title == "❌ Error"
    assert embed.description == description
    assert embed.color == discord.Color.red()


@pytest.mark.asyncio
async def test_base_view_on_error_uses_error_embed_response_send_message(monkeypatch):
    """BaseView.on_error should use error_embed and response.send_message when response is not done."""
    # Arrange
    sentinel_embed = discord.Embed(title="sentinel")

    def fake_error_embed(*args, **kwargs):
        return sentinel_embed

    # Patch where BaseView uses error_embed, not the test module import
    monkeypatch.setattr("capy_discord.ui.view.error_embed", fake_error_embed)

    interaction = MagicMock(spec=discord.Interaction)

    interaction.response = AsyncMock()
    interaction.response.is_done.return_value = False
    interaction.response.send_message = AsyncMock()

    interaction.followup = AsyncMock()
    interaction.followup.send = AsyncMock()

    view = BaseView()

    # Act
    await view.on_error(RuntimeError("boom"), item=None, interaction=interaction)

    # Assert
    interaction.response.send_message.assert_awaited_once_with(
        embed=sentinel_embed,
        ephemeral=True,
    )
    interaction.followup.send.assert_not_awaited()


@pytest.mark.asyncio
async def test_base_view_on_error_uses_error_embed_followup_send(monkeypatch):
    """BaseView.on_error should use error_embed and followup.send when response is already done."""
    # Arrange
    sentinel_embed = discord.Embed(title="sentinel")

    def fake_error_embed(*args, **kwargs):
        return sentinel_embed

    monkeypatch.setattr("capy_discord.ui.view.error_embed", fake_error_embed)

    interaction = MagicMock(spec=discord.Interaction)

    interaction.response = AsyncMock()
    interaction.response.is_done.return_value = True
    interaction.response.send_message = AsyncMock()

    interaction.followup = AsyncMock()
    interaction.followup.send = AsyncMock()

    view = BaseView()

    # Act
    await view.on_error(RuntimeError("boom"), item=None, interaction=interaction)

    # Assert
    interaction.followup.send.assert_awaited_once_with(
        embed=sentinel_embed,
        ephemeral=True,
    )
    interaction.response.send_message.assert_not_awaited()

```

1. If `BaseView` lives in a different module than `capy_discord.ui.view`, update the import and both `monkeypatch.setattr` targets accordingly.
2. If your test suite uses a different async test marker (e.g. `pytest.mark.anyio`), adjust the `@pytest.mark.asyncio` decorators to match your configuration.
3. If `BaseView.on_error` has a different signature or additional required parameters, update the `await view.on_error(...)` calls to match that signature.
</issue_to_address>